General tweaks & actual code generation for `int f() { return 5; }` #7

Jpnock · 2024-01-30T22:30:48Z

What does this PR do?

Comments on ASAN

Since ASAN (AddressSanitizer) is turned on, segfaults can easily be diagnosed by reading the log output from stderr (saved into the bin/output directory when you run ./test.sh). This gives a nice stack trace with the problematic line number.

Students will only see this if they navigate to the log file for the test (e.g. the first file in the console output below). In other words, they won't likely see it until they start hunting around the log files when they're having problems getting a test to pass.

To prevent ASAN from changing the compiler exit code, I've set ASAN_OPTIONS=exitcode=0 to allow the tests in test.sh to still be attempted even if these logs are produced.

==1859841==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000
==1859841==The signal is caused by a READ memory access.
==1859841==Hint: address points to the zero page.
    #0 in ReturnStatement::emitRISC(std::ostream&, Context&) const src/ast_statements.cpp:14
    #1 in NodeList::emitRISC(std::ostream&, Context&) const include/ast_node.hpp:51
    #2 in FunctionDefinition::emitRISC(std::ostream&, Context&) const src/ast_function_definition.cpp:18
    #3 in main src/compiler.cpp:47

Makes debugging errors easier

This is the same as test_RETURN but with constant 5678 instead of 10

…anging test from stalling the rest

Directives were also moved to FunctionDefinition as there was a bit too much code in main()

Fiwo735

Yet another great set of changes! The repo is getting more and more professional, especially for a coursework 😄 I especially like providing the two ways of going about the grammar (simplified vs full) as it should give the students an interesting design choice.

I have a few suggestions, which can be summarised as (please make sure I marked all the relevant code snippets):

using snake_case for variable names to easily distinguish them from functions and classes also somewhat adhere to Google's style guide
keeping the implementations of emitRISC (and the newly introduced print for that matter) in separate .cpp files as that's a general good practice and it would be consistent with other emitRISC implementations (which will especially make a lot of sense in cases where the implementation will require dozens of code lines)
making all emitRISC and print functions consistently const override for reminding students about inheritance and having a compiler do a sanity check as well.
using initializer list when applicable

I also wanted to discuss some things:

Do you think we should keep a reminder in every emitRISC and print function we provide that it's a simple dummy version and they should change it? Or just mention that in the specs (which I think we already do) and keep the code a bit cleaner?
I'm not sure I see the point of replacing specific jump_statement (.hpp/.cpp) with statements (.hpp/.cpp) that I assume would include definitions and implementations of all kinds of statements. To me it seems more error-prone and less consistent with how other .cpp/.hpp relate to ast nodes.
Given the new simplified grammar, I'd say we actually go a step further and delete some of the "chains" of rules that introduce nothing (... shift_expression -> additive_expression -> multiplicative_expression -> cast_expression ... etc.) in order to make the grammer even less daunting while giving the students complete freedom in how they go about implementing features should they choose the simple grammar as the starting point. In a way that would further cater students who don't want to fully understand the official grammar and just want to make small steps towards the featutes that seem feasible at the time to them.

include/ast_function_definition.hpp

include/ast_constant.hpp

include/ast_direct_declarator.hpp

src/compiler.cpp

Fiwo735 · 2024-01-31T02:44:07Z

Btw, would you mind removing "[WIP]" from somewhere in the changelog, as I forgot and wanted to avoid creating a new pull request just for that? And also, another pull request from a student happened in parallel to yours and there seems to be overlap - would you mind committing theirs idea (quite small) here, so that we could avoid a merge conflict?

This is made clear in the docs already

Jpnock · 2024-01-31T04:07:10Z

Yet another great set of changes! The repo is getting more and more professional, especially for a coursework 😄 I especially like providing the two ways of going about the grammar (simplified vs full) as it should give the students an interesting design choice.

Thanks for the review :)

Top four suggestions have been implemented and the [WIP] leftover has been removed. I've also created a CONTRIBUTING.md based on this which summarises the rules we now follow from the Google style guide.

Stderr Warnings

I've omitted the not implemented warnings as you suggested since this is already clear in the docs.

Long grammar chain

As for the long chain in the grammar, I had a think about this and thought it was probably better to keep. I say this for two reasons:

It means there are no actual changes between the simplified grammar and the full grammar, other than deletions. I think this will make adding things back one at a time easier than if they have to start figuring out where changes were made to it and reverting our changes when they add more rules.
I think it's also easier to introduce them to this chain now where they can just click on each non-terminal and follow it directly up, whereas it's much harder to see how this chain works in the full grammar since it no longer fits in the height of your window.

I did move it to the bottom of the grammar file though so it's seen last when going through the grammar.

Jump statement file

I've renamed from ast_statements.(hpp|cpp) to ast_jump_statement.(hpp|cpp). I think it's fine to have all the jump statements in there (e.g. ReturnStatement, ContinueStatement) but yeah you're right that if it's every statement in the AST it's going to be way too many.

Jpnock added 25 commits January 29, 2024 19:31

gitignore: Ignore code coverage output

d2e55a3

gitignore: Ignore compiler dependency files

df20f1c

docker: Install lcov dependency

b8ff059

Correct CPPFLAGS to CXXFLAGS

72e4138

make: Clean more generated files

3d61dc2

docs: General tweaks

02786d5

test: Exit early if make fails

801d284

test: Preserve assembly from toolchain test

da69108

Makes debugging errors easier

test: Improve output and logging of the test script

9e85b13

ast: Add a pretty print method for visualising the AST

1751376

Improve clarity of CLI arg parsing & output to pretty print file

fe9b2d3

docs: Update

1a0306b

Tidy grammar

ecc15c2

make: Tidy Makefile

2156b5e

ast: Prevent nullptr deref in FunctionDefinition when emitting body

1a572e4

Enable ASAN for easier debugging

4546d60

test: Tidy output of test script

039110a

Delete duplicate seen test

a72b5a0

This is the same as test_RETURN but with constant 5678 instead of 10

docker: Add nano for terminal commands that need a text editor

9e55f18

Implement support for return INT_CONSTANT; statements

71fb1f8

Simplify the grammar to the minimum example for int f() { return 5; }

c5bbf80

Rename the full grammar example

f4d9999

Add docs explaining the grammar files provided

90c8d4b

Change the example test back to return 5;

0455859

test: Add 15 second timeout to each stage of testing to prevent one h…

61c4efc

…anging test from stalling the rest

Jpnock marked this pull request as ready for review January 30, 2024 22:30

Jpnock marked this pull request as draft January 30, 2024 22:31

Jpnock added 3 commits January 30, 2024 22:47

Improve readability of FunctionDefinition

13fa148

Directives were also moved to FunctionDefinition as there was a bit too much code in main()

Avoid use of branches in Node as it hinders readability

5e78daa

Add a helper class, NodeList, and demonstrate its usage

bc0b229

Jpnock added 4 commits January 30, 2024 23:10

Improve Context docs

38242ed

Emit TODOs to stderr

1feca4e

Fix running the coverage command via test.sh

8f1f1d9

docs: Add instructions on how to view the coverage webpage

033870e

Jpnock marked this pull request as ready for review January 30, 2024 23:52

Jpnock added 2 commits January 31, 2024 00:09

Refactor compiler main()

bc1b25d

Enable coverage via env-var

7c93acf

Fiwo735 reviewed Jan 31, 2024

View reviewed changes

Jpnock added 2 commits January 31, 2024 03:03

Remove WIP comment

7cc832c

Comply with Google C++ Style Guide naming conventions

fe35d61

Jpnock force-pushed the jpnock/2024-further-tweaks branch from 958d3f3 to fe35d61 Compare January 31, 2024 03:33

Jpnock added 5 commits January 31, 2024 03:34

Separate headers and implementation

9458ef7

Added style guide suggestions for contributors

7fd6bd6

Update style guide

730ca0a

Remove not implemented warnings

61503c7

This is made clear in the docs already

Rename ast_statements to ast_jump_statement

7cd8fab

test: Don't run make clean if DONT_CLEAN=1

1034f7b

Fiwo735 approved these changes Feb 5, 2024

View reviewed changes

Fiwo735 merged commit d629d2f into main Feb 6, 2024
2 checks passed

Fiwo735 deleted the jpnock/2024-further-tweaks branch February 6, 2024 21:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

General tweaks & actual code generation for `int f() { return 5; }` #7

General tweaks & actual code generation for `int f() { return 5; }` #7

Jpnock commented Jan 30, 2024 •

edited

Loading

Fiwo735 left a comment

Fiwo735 commented Jan 31, 2024 •

edited

Loading

Jpnock commented Jan 31, 2024

General tweaks & actual code generation for int f() { return 5; } #7

General tweaks & actual code generation for int f() { return 5; } #7

Conversation

Jpnock commented Jan 30, 2024 • edited Loading

What does this PR do?

Comments on ASAN

Fiwo735 left a comment

Choose a reason for hiding this comment

Fiwo735 commented Jan 31, 2024 • edited Loading

Jpnock commented Jan 31, 2024

Stderr Warnings

Long grammar chain

Jump statement file

General tweaks & actual code generation for `int f() { return 5; }` #7

General tweaks & actual code generation for `int f() { return 5; }` #7

Jpnock commented Jan 30, 2024 •

edited

Loading

Fiwo735 commented Jan 31, 2024 •

edited

Loading